Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Format all code #1441

Merged
merged 4 commits into from
Jun 2, 2022
Merged

Format all code #1441

merged 4 commits into from
Jun 2, 2022

Conversation

nojaf
Copy link
Contributor

@nojaf nojaf commented Jun 1, 2022

Hello, would you be interested in using Fantomas for this community project?

@nojaf
Copy link
Contributor Author

nojaf commented Jun 1, 2022

Fantomas is net6 only thing, is there any reason this project is still on net5?

@baronfel
Copy link
Contributor

baronfel commented Jun 1, 2022

even if there is a reason for sticking to net5.0 target frameworks, the project itself can and should move to 6.0 SDKs. You can target older TFMs from newer SDKs, you just also get the benefit of any fixes/feature enhancements that came with the newer SDK.

@nojaf
Copy link
Contributor Author

nojaf commented Jun 1, 2022

Right, so I should be able to bump the global json without many consequences right?

@baronfel
Copy link
Contributor

baronfel commented Jun 1, 2022

Yes, with a caveat - the PR/merge workflows use the setup-dotnet action to ensure the appropriate version of the .NET SDK is installed. If you change the value in global.json, I would also change these usages over to install multiple .NET SDKs to ensure that tests still run:

- name: Setup dotnet
  uses: actions/setup-dotnet@v2
  with:
    dotnet-version: | 
      5.0.x
      6.0.x

@nojaf
Copy link
Contributor Author

nojaf commented Jun 1, 2022

Right on! Thanks @baronfel.
@cartermp and @dsyme let me know what you prefer in this case.

@dsyme
Copy link
Contributor

dsyme commented Jun 2, 2022

Reformatting may be needed after pull

Regarding .NET version - we can update it - it might require an update to paket and fake?

maxNumberOfRows
=

using (logTime "LoadingTextToBeParsed" valueToBeParsedOrItsUri)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will pre-change all explicit using to use _holder = ... to prevent this indentation happening. Then we reformat

@cartermp
Copy link
Collaborator

cartermp commented Jun 2, 2022

Updating to net6 is possible, it will just be annoying. IIRC this requires FAKE to move to be project-based

typeof<int>
typeof<int64> ]
typeof<float>,
[ typeof<Bit0>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please allow maximum list size large enough to allow these to be formatted single line

let ctor =
ProvidedConstructor(
[ for field in fields -> field.ProvidedParameter ],
invokeCode =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These invokeCode expand out a fair bit, I will pre-adjust them to be inner function bindings

text,
LoadOptions.PreserveWhitespace
)
.Root
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is strangely expanded. Is line width 80? I think it could be set to 120.

@dsyme
Copy link
Contributor

dsyme commented Jun 2, 2022

@nojaf Could you reformat after #1442 please?

@cartermp We should likely just cut FAKE out of this project

@dsyme
Copy link
Contributor

dsyme commented Jun 2, 2022

@nojaf I'm doing the update to .NET 6 separately here: #1443

@dsyme
Copy link
Contributor

dsyme commented Jun 2, 2022

@nojaf I'll do the merge and the things I mentioned above, and re-format

@dsyme
Copy link
Contributor

dsyme commented Jun 2, 2022

OK this is now ready - I'm happy with the code after formatting, more or less

@dsyme
Copy link
Contributor

dsyme commented Jun 2, 2022

I'll make a couple more tweaks to .edtiorconfig in a separate PR

@dsyme dsyme merged commit 8639894 into fsprojects:main Jun 2, 2022
@dsyme
Copy link
Contributor

dsyme commented Jun 2, 2022

@nojaf Thank you sooooooo much for pushing this along. It makes the repository so much easier to maintain and contribute to

@nojaf
Copy link
Contributor Author

nojaf commented Jun 3, 2022

You are most welcome. The more projects are formatted, the more exposure for Fantomas 😸.

@nojaf nojaf deleted the format branch June 3, 2022 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants